Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add eventbus to cloudwatch event rule #15727

Merged
merged 14 commits into from
Oct 27, 2020

Conversation

gdavison
Copy link
Contributor

Completes work from #12886 to add custom event buses to CloudWatch Events rules.

Closes #12886
Relates #9330

Release note for CHANGELOG:

* resource/aws_cloudwatch_event_rule: Add `event_bus_name`

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSCloudWatchEventRule_'

--- PASS: TestAccAWSCloudWatchEventRule_Name_Generated (119.30s)
--- PASS: TestAccAWSCloudWatchEventRule_NamePrefix (124.29s)
--- PASS: TestAccAWSCloudWatchEventRule_ScheduleAndPattern (124.30s)
--- PASS: TestAccAWSCloudWatchEventRule_role (130.35s)
--- PASS: TestAccAWSCloudWatchEventRule_description (165.74s)
--- PASS: TestAccAWSCloudWatchEventRule_pattern (167.54s)
--- PASS: TestAccAWSCloudWatchEventRule_IsEnabled (198.77s)
--- PASS: TestAccAWSCloudWatchEventRule_basic (198.91s)
--- PASS: TestAccAWSCloudWatchEventRule_EventBusName (201.43s)
--- PASS: TestAccAWSCloudWatchEventRule_tags (223.86s)

@gdavison gdavison requested a review from a team October 20, 2020 05:57
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. provider Pertains to the provider itself, rather than any interaction with AWS. service/cloudwatchevents tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Oct 20, 2020
@awendt
Copy link

awendt commented Oct 26, 2020

@gdavison Thanks for working on this. We created a fork based on 3.12.0, merged in your changes from this PR (and resource_aws_cloudwatch_event_target) but noticed we cannot import rules that are defined on an event bus different from default. Here's a minimal config:

provider "aws" {
  version = "3.12.0-with-15727-and-15799"

  region = "eu-west-1"
}

data "aws_caller_identity" "current" {}

resource "aws_cloudwatch_event_bus" "test" {
  name = "test-awendt"
}

resource "aws_cloudwatch_event_rule" "non-default-bus" {
  event_bus_name = aws_cloudwatch_event_bus.test.name

  description    = "Filters all events from EventBridge Bus ${aws_cloudwatch_event_bus.test.name}"
  is_enabled     = false
  name           = "test-rule"
  event_pattern  = jsonencode(
    {
      account = [
        data.aws_caller_identity.current.account_id,
      ]
    }
  )
}

Steps to reproduce

  1. Create the event bus using AWS CLI: aws events create-event-bus --name test-awendt
  2. Create an event rule on that bus: aws events put-rule --name test-rule --state DISABLED --event-pattern "{\"account\": [\"AWS_ACCOUNT_ID\"]}" --event-bus-name test-awendt (you need to interpolate the account ID)
  3. Import the event bus: terraform import aws_cloudwatch_event_bus.test test-awendt
  4. Import the event rule: terraform import aws_cloudwatch_event_rule.non-default-bus test-rule

This last step fails with:

aws_cloudwatch_event_rule.non-default-bus: Importing from ID "test-rule"...
aws_cloudwatch_event_rule.non-default-bus: Import prepared!
  Prepared aws_cloudwatch_event_rule for import
aws_cloudwatch_event_rule.non-default-bus: Refreshing state... [id=test-rule]

Error: Cannot import non-existent remote object

While attempting to import an existing object to
aws_cloudwatch_event_rule.non-default-bus, the provider detected that no
object exists with the given id. Only pre-existing objects can be imported;
check that the id is correct and that it is associated with the provider's
configured region or endpoint, or use "terraform apply" to create a new remote
object for this resource.

Can you reproduce this? What could be causing this behavior?

@awendt
Copy link

awendt commented Oct 26, 2020

@gdavison Some more context: We noticed Terraform wanted to create a resource that was already existing. It seems both refresh and import are not working as intended.

@gdavison
Copy link
Contributor Author

Hi @awendt. When importing a rule for a non-default bus, the format of the import id is /, and it assumes that the bus is the default bus if only the is passed. In the example you posted, you'd need to call terraform import aws_cloudwatch_event_rule.non-default-bus test-awendt/test-rule.

@bflad bflad self-assigned this Oct 26, 2020
@bflad bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Oct 26, 2020
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small question, otherwise LGTM 🚀

Output from acceptance testing:

--- PASS: TestAccAWSCloudWatchEventRule_ScheduleAndPattern (34.58s)
--- PASS: TestAccAWSCloudWatchEventRule_Name_Generated (38.15s)
--- PASS: TestAccAWSCloudWatchEventRule_NamePrefix (39.13s)
--- PASS: TestAccAWSCloudWatchEventRule_role (53.03s)
--- PASS: TestAccAWSCloudWatchEventRule_pattern (56.78s)
--- PASS: TestAccAWSCloudWatchEventRule_description (58.19s)
--- PASS: TestAccAWSCloudWatchEventRule_IsEnabled (68.64s)
--- PASS: TestAccAWSCloudWatchEventRule_basic (71.60s)
--- PASS: TestAccAWSCloudWatchEventRule_EventBusName (72.20s)
--- PASS: TestAccAWSCloudWatchEventRule_tags (82.68s)

Comment on lines 306 to 308
} else {
return nil, errors.New("schedule_expression can only be set on the default event bus")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Is the API error not clear in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is short-circuiting the API call. I would have set it as a ValidateFunc if they supported cross-attribute validation

@awendt
Copy link

awendt commented Oct 27, 2020

@gdavison Thanks for catching this, I can confirm the import works as you described. 🎉

I was also able to track down the root cause why refresh isn't working for us, and I believe we can just re-import the resources and all will be good.

@gdavison gdavison merged commit 49d6d9f into master Oct 27, 2020
@gdavison gdavison deleted the add-eventbus-to-cloudwatch-event-rule branch October 27, 2020 19:42
@breathingdust breathingdust modified the milestones: Roadmap, v3.13.0 Nov 2, 2020
@ghost
Copy link

ghost commented Nov 27, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. provider Pertains to the provider itself, rather than any interaction with AWS. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants